Skip to content

Unify reproducible benchmark result storage#306

Merged
berntpopp merged 38 commits into
mainfrom
chore/benchmark-result-store
Jul 13, 2026
Merged

Unify reproducible benchmark result storage#306
berntpopp merged 38 commits into
mainfrom
chore/benchmark-result-store

Conversation

@janpower

@janpower janpower commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add a shared result store for retrieval, extraction, and LLM benchmarks
  • preserve collision-free run directories with manifests, checksums, canonical term/case artifacts, and legacy compatibility
  • add CSC/GSC LLM result support, checkpoint identity validation, recursive artifact discovery, and comprehensive tests
  • bind checkpoint reuse to dataset contents and accounting configuration
  • retain deprecated LLM output paths during migration and record non-evaluable retrieval cases explicitly

Motivation

Benchmark outputs previously used overlapping flat paths and inconsistent artifact formats. This made repeated runs, checkpoint resume, cross-model comparison, and downstream manuscript analysis difficult to reproduce reliably.

This PR contains only reusable Phentrieve benchmark infrastructure, tests, and technical user documentation. Manuscript-specific analyses, run data, logs, figures, and planning documents are intentionally excluded.

Validation

  • make check
  • make typecheck-fast
  • uv sync --all-extras --frozen
  • make test (2023 passed, 48 skipped)
  • focused benchmark regression suite (85 passed)

janpower and others added 27 commits July 12, 2026 22:42
…out kwargs

Update all run_llm_benchmark_cli() calls in the integration test file from the
retired output_path/checkpoint_path/artifacts_dir kwargs to output_dir/run_id,
and read persisted artifacts back from the RunLayout-derived paths
(run_dir/summary.json, predictions_dir, cases.jsonl) instead of the old
single output_path file. Covers 5 tests flagged by review plus 4 more in the
same file with the same root cause that were missed because they don't match
"llm" in their test names.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tifacts-dir with --output-dir/--run-id/--overwrite
…ifests

terms.jsonl/cases.jsonl are only computed once at completion, so a
partial-run manifest's counts.terms/counts.cases previously always
read 0 while a checkpoint was still in progress.
Run Artifacts and LLM Full-Text Benchmark sections still described the
pre-unification results/llm/<timestamp>.json layout and a stale output
JSON key list.
… matches

_build_benchmark_payload never wrote capture_phase1_debug into the
checkpoint, so _checkpoint_matches_run compared the stored default
(None) against the CLI's real default (False) and always rejected the
checkpoint on any real --run-id/--overwrite resume. Discovered live
while resuming an interrupted OpenRouter run.
…/base_url

current_run compared the raw --llm-provider/--llm-base-url CLI inputs,
but run_llm_benchmark() persists the provider resolver's resolved
values (e.g. the default provider when --llm-provider is omitted, or
the model-prefix-inferred provider). Resolve once in the CLI so a
resume with the same omitted flags matches instead of always rejecting
the checkpoint.
Checkpoints written before capture_phase1_debug started being
persisted still lack the key entirely, so a resume against one of
those legacy files failed the same match check the previous fix
addressed for newly-written checkpoints. Discovered resuming a real
interrupted run whose checkpoint predated that fix.
@janpower

Copy link
Copy Markdown
Collaborator Author

Independent review findings addressed in ec70549: dataset-content and accounting fingerprints now gate checkpoint reuse; deprecated output paths remain supported with warnings; retrieval cases without gold IDs are recorded as not_evaluable. Full suite: 2023 passed, 48 skipped; check and typecheck pass.

@janpower
janpower requested a review from berntpopp July 12, 2026 21:26
@janpower
janpower marked this pull request as ready for review July 12, 2026 21:35
dependabot Bot and others added 10 commits July 13, 2026 07:07
Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 8.1.3 to 8.1.4.
- [Release notes](https://github.com/vitejs/vite/releases)
- [Changelog](https://github.com/vitejs/vite/blob/main/packages/vite/CHANGELOG.md)
- [Commits](https://github.com/vitejs/vite/commits/v8.1.4/packages/vite)

---
updated-dependencies:
- dependency-name: vite
  dependency-version: 8.1.4
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
…tory with 8 updates

Bumps the frontend-minor-patch group with 6 updates in the /frontend directory:

| Package | From | To |
| --- | --- | --- |
| [libphonenumber-js](https://gitlab.com/catamphetamine/libphonenumber-js) | `1.13.7` | `1.13.8` |
| [@vitest/coverage-v8](https://github.com/vitest-dev/vitest/tree/HEAD/packages/coverage-v8) | `4.1.9` | `4.1.10` |
| [eslint](https://github.com/eslint/eslint) | `10.6.0` | `10.7.0` |
| [postcss](https://github.com/postcss/postcss) | `8.5.16` | `8.5.18` |
| [prettier](https://github.com/prettier/prettier) | `3.9.4` | `3.9.5` |
| [terser](https://github.com/terser/terser) | `5.48.0` | `5.49.0` |



Updates `libphonenumber-js` from 1.13.7 to 1.13.8
- [Changelog](https://gitlab.com/catamphetamine/libphonenumber-js/blob/master/CHANGELOG.md)
- [Commits](https://gitlab.com/catamphetamine/libphonenumber-js/commits/master)

Updates `@vitest/coverage-v8` from 4.1.9 to 4.1.10
- [Release notes](https://github.com/vitest-dev/vitest/releases)
- [Changelog](https://github.com/vitest-dev/vitest/blob/main/docs/releases.md)
- [Commits](https://github.com/vitest-dev/vitest/commits/v4.1.10/packages/coverage-v8)

Updates `@vitest/ui` from 4.1.9 to 4.1.10
- [Release notes](https://github.com/vitest-dev/vitest/releases)
- [Changelog](https://github.com/vitest-dev/vitest/blob/main/docs/releases.md)
- [Commits](https://github.com/vitest-dev/vitest/commits/v4.1.10/packages/ui)

Updates `eslint` from 10.6.0 to 10.7.0
- [Release notes](https://github.com/eslint/eslint/releases)
- [Commits](eslint/eslint@v10.6.0...v10.7.0)

Updates `postcss` from 8.5.16 to 8.5.18
- [Release notes](https://github.com/postcss/postcss/releases)
- [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md)
- [Commits](postcss/postcss@8.5.16...8.5.18)

Updates `prettier` from 3.9.4 to 3.9.5
- [Release notes](https://github.com/prettier/prettier/releases)
- [Changelog](https://github.com/prettier/prettier/blob/main/CHANGELOG.md)
- [Commits](prettier/prettier@3.9.4...3.9.5)

Updates `terser` from 5.48.0 to 5.49.0
- [Changelog](https://github.com/terser/terser/blob/master/CHANGELOG.md)
- [Commits](terser/terser@v5.48.0...v5.49.0)

Updates `vitest` from 4.1.9 to 4.1.10
- [Release notes](https://github.com/vitest-dev/vitest/releases)
- [Changelog](https://github.com/vitest-dev/vitest/blob/main/docs/releases.md)
- [Commits](https://github.com/vitest-dev/vitest/commits/v4.1.10/packages/vitest)

---
updated-dependencies:
- dependency-name: libphonenumber-js
  dependency-version: 1.13.8
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: frontend-minor-patch
- dependency-name: "@vitest/coverage-v8"
  dependency-version: 4.1.10
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: frontend-minor-patch
- dependency-name: "@vitest/ui"
  dependency-version: 4.1.10
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: frontend-minor-patch
- dependency-name: eslint
  dependency-version: 10.7.0
  dependency-type: direct:development
  update-type: version-update:semver-minor
  dependency-group: frontend-minor-patch
- dependency-name: postcss
  dependency-version: 8.5.18
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: frontend-minor-patch
- dependency-name: prettier
  dependency-version: 3.9.5
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: frontend-minor-patch
- dependency-name: terser
  dependency-version: 5.49.0
  dependency-type: direct:development
  update-type: version-update:semver-minor
  dependency-group: frontend-minor-patch
- dependency-name: vitest
  dependency-version: 4.1.10
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: frontend-minor-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
…produce

Two defects in the shared result store, both of which undermine the
reproducibility guarantee the store exists to provide.

`create_run_layout(overwrite=True)` reused a run directory without clearing it.
`write_manifest` decides what to advertise purely from `Path.exists()`, so a
rerun under the same `--run-id` inherited the previous run's files and published
them as its own: re-running an extraction without diagnostics still advertised
the earlier `diagnostics/chunks.jsonl`, and a narrower `--doc-ids` LLM rerun left
the earlier documents inside the advertised `predictions/` directory. Clear the
run directory on overwrite, preserving only `checkpoint.json` — the LLM benchmark
reuses a run directory precisely to resume from it, and that checkpoint's
identity is validated separately before any of it is trusted. The checkpoint
filename is now owned by `RunLayout` instead of being duplicated as a literal.

`discover_artifacts` was also type-blind. All three benchmark types write a
`summary` role into the shared results root, so the recursive discovery handed
extraction and LLM summaries to the two loaders that render dense-retrieval
tables. Those summaries carry no `mrr_dense`, so `phentrieve benchmark compare`
listed them as models scoring `MRR (Dense) = 0`, and every new extraction or LLM
run added another junk row. Add an optional `benchmark_type` filter and set it in
both retrieval loaders; the legacy fallback stays unfiltered because the old flat
layout only ever held retrieval summaries.

`load_summary_files` also swapped `glob.glob` (str) for `discover_artifacts`
(Path) without updating the `file_path` it injects into each summary, which broke
JSON serialization of loaded summaries.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfjxrHsHYH2TqjwgEcq6oS
`cases.jsonl` mixed two identifier schemes. Evaluable and failed cases were
keyed on a 0-based `case_id` — the same key `terms.jsonl` joins on — while cases
skipped for having no gold terms were keyed on a 1-based `case_index` and carried
no `case_id` at all. A downstream join over the artifact therefore silently
dropped exactly the rows the manifest counts under `not_evaluable`, and the two
index bases sat in one file.

Emit `case_id` (and the case text) for non-evaluable rows too, so every row in
`cases.jsonl` is joinable on one stable key.

Also return `run_dir` as a str: it is part of a public result dict, and the LLM
payload already returns a str, so a caller serializing the retrieval result hit
`TypeError: Object of type PosixPath is not JSON serializable`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfjxrHsHYH2TqjwgEcq6oS
…tatus

The checkpoint identity recorded `prompt_templates_dir` but not the templates
themselves. The prompt is an experimental variable, so editing a template in
place between two runs of the same `--run-id` left the identity unchanged and let
the resume merge old-prompt and new-prompt document outputs into one set of
metrics. Hash the templates a run will actually load — both the packaged set and
the effective user set, which is `~/.phentrieve/prompts` even when the flag is
omitted — and make that hash part of the identity.

Checkpoints written before the key existed carry no evidence about the templates
they ran under, so they stay resumable under the behaviour they were written with
rather than being invalidated on upgrade; checkpoints written from now on always
carry the hash and are compared strictly.

The manifest also always claimed `status: complete`. `run_llm_benchmark` reports
`completed` for any run that finishes its document loop, including one in which
every document failed, and the CLI mapped anything not equal to `failed` onto
`complete`. Derive the run status from the per-case outcomes instead — as the
retrieval runner already does — and record failed/complete counts so the status
is auditable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfjxrHsHYH2TqjwgEcq6oS
Both new artifact writers open-coded `1.0 - distance` when the retriever returned
distances without similarities, while the retriever itself converts through
`calculate_similarity`, which clamps to [0, 1]. An unclamped score also feeds
`passes_threshold` in the extraction chunk diagnostics. Use the shared helper so
the artifacts agree with the scores the pipeline actually acted on.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfjxrHsHYH2TqjwgEcq6oS
The guide told users to pass `--summaries-dir results/retrieval` to work around
comparisons picking up non-retrieval runs; the loaders now filter by benchmark
type, so the plain results root is safe.

It also described `traces/` as written only with `--capture-phase1-debug`. The
prediction record always carries a trace, so traces are always written; the flag
adds phase-1 retrieval detail to them rather than enabling them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfjxrHsHYH2TqjwgEcq6oS
…/vite-8.1.4' into chore/benchmark-result-store
Merges the two open Dependabot frontend PRs into this branch. Both edit the same
dependency block, so package.json is resolved as the union of the two — vite
^8.1.4 from #307 alongside the six bumps from #308 (libphonenumber-js, eslint,
postcss, prettier, terser, @vitest/coverage-v8) — and package-lock.json is
regenerated from the resolved manifest rather than hand-merged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfjxrHsHYH2TqjwgEcq6oS
CLI takes a minor bump for the benchmark result store and its new flags; the
frontend takes a patch bump for the folded-in dependency updates. The API is
untouched by this change and keeps 0.17.0.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfjxrHsHYH2TqjwgEcq6oS
@berntpopp

Copy link
Copy Markdown
Owner

Deep review — full-stack + RAG/LLM lens, plus an independent Codex (gpt-5.5, xhigh) pass

Strong PR: the run-directory + manifest + checksum model is the right shape, _atomic_text_write is properly atomic, the CSC/GSC support is clean, and binding checkpoint reuse to the resolved provider/model (rather than raw CLI input) is a subtle call that's exactly right.

I found six defects and fixed them on the branch. Both reviews converged on the same top-two independently, which is a good signal they're real. Each is covered by a regression test.

Fixed

1. Overwritten runs inherited artifacts they never produced (high — this defeats the PR's core promise)
create_run_layout(overwrite=True) reused the directory without clearing it, and write_manifest decides what to advertise purely from Path.exists(). So run 2 published run 1's files as its own. Reproduced:

run 1 files: ['diagnostics/chunks.jsonl', 'manifest.json', 'predictions/doc_A.json', 'terms.jsonl']
run 2 (--overwrite, no diagnostics this time):
  manifest artifacts: ['term_results', 'chunk_diagnostics']   <-- claims chunks it never wrote
  chunk_diagnostics content: {"chunk_id":0,"run":1}           <-- run 1's data

Now the run directory is cleared on overwrite, preserving only checkpoint.json — the LLM benchmark reuses a directory precisely to resume from it, and that checkpoint's identity is validated separately. RunLayout owns the checkpoint filename now instead of it being a duplicated literal.

2. benchmark compare listed extraction and LLM runs as retrieval models (high)
All three benchmark types write a summary role into the shared results root, and discover_artifacts was type-blind. Every extraction/LLM run added a junk row to the dense-retrieval table:

      Model   Dataset  Run ID   MRR (Dense)
BAAI/bge-m3 pheno_test   ret1          0.71
    Unknown   GSC_plus   ext1          0.00   <-- extraction run
    Unknown        CSC   llm1          0.00   <-- LLM run

discover_artifacts takes an optional benchmark_type filter; both retrieval loaders set it. (The guide worked around this with --summaries-dir results/retrieval; the default is now correct, so I updated it.)

3. Checkpoint identity didn't cover the prompt templates (high — RAG-specific)
Identity recorded prompt_templates_dir but not its contents. The prompt is an experimental variable: edit a template in place, resume the same --run-id, and doc 1's old-prompt output silently merges with doc 2's new-prompt output into one set of metrics. The PR already hashes the dataset — prompts deserve the same treatment. Now hashed (packaged + effective user set, which is ~/.phentrieve/prompts even without the flag). Checkpoints predating the key stay resumable rather than being invalidated on upgrade; ones written from now on are compared strictly.

4. LLM manifests always claimed status: complete (high)
run_llm_benchmark returns status="completed" for any run that finishes its loop — including one where every document failed — and the CLI mapped anything != failed to complete. Status is now derived from per-case outcomes (complete/partial/failed), as the retrieval runner already did, with failed/complete counts in the manifest.

5. cases.jsonl mixed two key schemes (medium)
Evaluable/failed cases used 0-based case_id (what terms.jsonl joins on); non-evaluable cases used 1-based case_index and no case_id — so a downstream join silently dropped exactly the rows the manifest counts under not_evaluable. Unified on case_id.

6. Path leaked into public result dicts (medium)
run_evaluation() returned run_dir as a Path (the LLM payload already returned str), and swapping glob.globdiscover_artifacts flipped result_analyzer's injected file_path from str to Path. Either breaks a caller that JSON-serializes the result. Both now str.

Also: the distance→similarity fallback open-coded 1.0 - d while the retriever converts through calculate_similarity (which clamps to [0,1]); an unclamped score feeds passes_threshold in the chunk diagnostics. Switched to the shared helper. And traces/ are always written, not gated on --capture-phase1-debug as the guide claimed — doc corrected.

Checked, no finding

Retrieval scoring semantics are sound: the store is ChromaDB cosine, DenseRetriever returns similarities, and 1 - d matches the existing aggregation.py convention. discover_artifacts deliberately ignores stray flat *_summary.json once canonical runs exist — that's an explicit, tested decision, so I left it alone.

Also folded in

Dependabot #307 (vite 8.1.4) and #308 (8 frontend deps). Both edit the same block, so package.json is the union of the two and the lockfile was regenerated from it. The prettier 3.8.4 → 3.9.5 bump reformatted nothing.

Versions

chore(release): 0.26.0 (CLI 0.26.0 / API 0.17.0 / Frontend 0.17.1) — CLI minor for the result store and new flags, frontend patch for the dependency bumps. API is untouched by this PR and stays at 0.17.0.

Verified locally: make ci-local green (Python 2030 passed / 78% coverage; frontend 326 passed, lint + format + build), make security-python clean (6 pre-existing LOW findings, all in untouched files).

🤖 Generated with Claude Code

pip-audit went red on this branch without a dependency change of ours: the click
and pillow advisories were published after the branch's last green run, and
main carries the identical versions. Both have patched releases, so they are
upgraded rather than added to the ignore list (which is reserved for advisories
with no fix available).

- click 8.3.2 -> 8.4.2 (PYSEC-2026-2132)
- pillow 12.2.0 -> 12.3.0 (PYSEC-2026-2253 through -2257)

typer stays on 0.25.1; the lazy CLI subgroups were verified to still populate
after the click minor bump.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GfjxrHsHYH2TqjwgEcq6oS
@berntpopp
berntpopp merged commit 4c59ae3 into main Jul 13, 2026
16 checks passed
@berntpopp
berntpopp deleted the chore/benchmark-result-store branch July 13, 2026 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants